[OutputManager] Removes use_name report filter option.#2876
Conversation
|
Current Coverage: 99% Mypy errors on eval-use-name branch: 1219 |
|
🚨 Please update the changelog. This PR cannot be merged until |
|
Current Coverage: 99% Mypy errors on eval-use-name branch: 1213 |
| base_info_map = { | ||
| "class": EnergyEstimator.__name__, | ||
| "function": EnergyEstimator.estimate_all.__name__, | ||
| "function": EnergyEstimator.report_diesel_consumption.__name__, |
There was a problem hiding this comment.
This was the incorrect function name previously and changing it will change existing filters using regex patterns with the old function name. I will update dev team channel once this PR is approved.
matthew7838
left a comment
There was a problem hiding this comment.
LGTM, the outputs were also the same after the changes on my side. Left a minor comment. Great work, Niko.
JoeWaddell
left a comment
There was a problem hiding this comment.
Tested with existing report filters, including those that used the use_name field, and all works as expected.
There are some slight differences in reported values, but on the order of <0.3% differences, which I concluded are likely simply due to random number generation (I was using the same input files for both, the same seed, but if there are different numbers of operations then some events will unfold differently, regardless of the seed being identical). The small differences were across multiple modules, so not restricted to the more directly affected fields here.
|
Current Coverage: 99% Mypy errors on eval-use-name branch: 1214 |
|
Current Coverage: 99% Mypy errors on eval-use-name branch: 1212 |
Evaluated
use_nameoutput filter option and determined it was no longer needed.Context
Issue(s) closed by this pull request: closes #2778
What
use_nameinOutputManagerandDieselConsumption.DieselConsumptionreporting function that relied onuse_nameformatting to reflect use of default filtering and data parsing inOutputManager(in this case because the output is in a dictionary format it will use the full variable name address for each dictionary element).Why
The
use_nameoption was helpful for parsing and filtering variables nested in dictionaries and being able to distinguish their values properly. With refactors done inOutputManagerthat now consistently provide the full variable address for those nested variables, it is a more reliable filtering/parsing process. Leaving theuse_nameoption available actually made that process now less reliable because it was performing a similar role but in a slightly different manner. Removing it to avoid confusion seemed the best plan.How
Removed all references to
use_namefromOutputManagerandDieselConsumption.DieselConsumptionrelied on this output parsing process to properly distinguish between these nested field variables. So I refactored the parsing process forDieselConsumptionto use the now universally default verbose variable naming process for nested variables.Test plan
The main thing to test is whether
DieselConsumptionis still reporting the same as it was ondev. I used this filter for that on this feature branch:{ "direction": "landscape", "multiple": [ { "name": "ShaqDiesel", "filters": [ ".*total_diesel_consumption_tractor_implement.*", ".*EnergyEstimator.report_diesel_consumption.*" ] } ] }Compare that to the results on
devusing a similar filter but slightly different because the function name in the info map was incorrect:{ "direction": "landscape", "multiple": [ { "name": "ShaqDiesel", "filters": [ ".*total_diesel_consumption_tractor_implement.*", ".*EnergyEstimator.estimate_all.*" ] } ] }Input Changes
Output Changes
Filter